Precompile advanced order#2685
Conversation
…e-advanced-orders
| /// subnet are skipped. | ||
| #[pallet::call_index(1)] | ||
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | ||
| pub fn execute_batched_orders( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders pulls TAO/staked alpha from every order signer into the pallet account via collect_assets, then makes later fallible calls (net_pool_swap, distribution transfers, zero-output checks). Without a transaction boundary, an error after collect_assets can return Err while leaving balances/stake already moved into or through the pallet account and orders not marked fulfilled. A public relayer can trigger this with valid signed orders plus a later slippage/zero-output/distribution failure, locking or misrouting user funds. Wrap the whole batched dispatch in a FRAME transaction so any later error rolls back the asset collection and intermediate transfers.
| pub fn execute_batched_orders( | |
| #[frame_support::transactional] | |
| pub fn execute_batched_orders( |
| let available = | ||
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | ||
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | ||
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | ||
| from_hotkey, | ||
| from_coldkey, | ||
| netuid, | ||
| amount, | ||
| ); | ||
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | ||
| to_hotkey, to_coldkey, netuid, amount, | ||
| ); | ||
| LastColdkeyHotkeyStakeBlock::<T>::insert( | ||
| to_coldkey, | ||
| to_hotkey, | ||
| Self::get_current_block_as_u64(), | ||
| ); | ||
| if validate_receiver { | ||
| ensure!( | ||
| Self::hotkey_account_exists(to_hotkey), | ||
| Error::<T>::HotKeyAccountNotExists | ||
| ); | ||
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | ||
| } |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
transfer_staked_alpha decreases the sender stake, increases receiver stake, and writes the rate-limit block before checking hotkey_account_exists(to_hotkey) when validate_receiver is true. In the new batched limit-order distribution path, a buy order with an unregistered destination hotkey can therefore make this helper return Err after stake has already been moved. Validate the receiver before any stake mutation, then set the receiver rate limit after the successful move.
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| } | |
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } |
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE MEDIUM scrutiny: open-junius has repo write permission and substantial prior activity; PR is multi-author and targets precompile-advanced-order -> devnet-ready. No trusted review prompt files or Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but the limit-order execution path still has a non-atomic batched settlement flow and a direct buy-order fee-evasion path. These are economic/state-safety vulnerabilities and should block merge. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| #[pallet::call_index(1)] | ||
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | ||
| pub fn execute_batched_orders( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders can transfer TAO/staked alpha into the pallet account in collect_assets, then return Err from net_pool_swap, distribute_alpha_pro_rata, or distribute_tao_pro_rata. Without a transaction boundary on the extrinsic, those earlier balance/stake mutations are not rolled back, so a failing batch can strand or move user assets while the order remains unfulfilled. Wrap the whole batched extrinsic in #[frame_support::transactional] or otherwise make every post-collection failure non-fatal and fully accounted.
| #[pallet::call_index(1)] | |
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | |
| pub fn execute_batched_orders( | |
| #[pallet::call_index(1)] | |
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | |
| #[frame_support::transactional] | |
| pub fn execute_batched_orders( |
| let available = | ||
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | ||
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | ||
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | ||
| from_hotkey, | ||
| from_coldkey, | ||
| netuid, | ||
| amount, | ||
| ); | ||
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | ||
| to_hotkey, to_coldkey, netuid, amount, | ||
| ); | ||
| LastColdkeyHotkeyStakeBlock::<T>::insert( | ||
| to_coldkey, | ||
| to_hotkey, | ||
| Self::get_current_block_as_u64(), | ||
| ); | ||
| if validate_receiver { | ||
| ensure!( | ||
| Self::hotkey_account_exists(to_hotkey), | ||
| Error::<T>::HotKeyAccountNotExists | ||
| ); | ||
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | ||
| } |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
When validate_receiver is true, this function decreases the sender stake, increases the receiver stake, and writes LastColdkeyHotkeyStakeBlock before checking hotkey_account_exists(to_hotkey). If that receiver check fails, the function returns an error after mutating stake accounting. In the current batched-order path this compounds the missing atomicity issue; the receiver-side validation needs to happen before any stake movement.
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| } | |
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| let pallet_hotkey = T::PalletHotkey::get(); | ||
|
|
||
| // Pull all input assets into the pallet intermediary before touching the pool. | ||
| Self::collect_assets( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders still starts by moving every buyer's TAO and seller's staked alpha into the pallet intermediary, then performs the net pool swap, pro-rata distributions, fee forwarding, order-status writes, and event emission as separate fallible steps. There is no outer #[transactional] / with_transaction boundary around the whole batch. If any later transfer or swap fails after this line, earlier collect_assets transfers are not rolled back, so users can lose assets while the extrinsic returns an error and some orders remain unfilled. Wrap the full do_execute_batched_orders asset-collection/swap/distribution/status-write flow in a transaction, or prevalidate every fallible post-collection transfer so no failure is possible after user assets are pulled.
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | ||
| from_hotkey, | ||
| from_coldkey, | ||
| netuid, | ||
| amount, | ||
| ); | ||
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | ||
| to_hotkey, to_coldkey, netuid, amount, | ||
| ); | ||
| LastColdkeyHotkeyStakeBlock::<T>::insert( | ||
| to_coldkey, | ||
| to_hotkey, | ||
| Self::get_current_block_as_u64(), | ||
| ); | ||
| if validate_receiver { |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
transfer_staked_alpha decreases the sender's stake, increases the receiver's stake, and writes LastColdkeyHotkeyStakeBlock before checking validate_receiver. The batch distributor calls this with validate_receiver = true for buyers, so a bad buyer hotkey can make the function return HotKeyAccountNotExists after stake has already been moved out of the pallet intermediary. Because this helper is not transactional and the outer batch is not transactional, that failure leaves mutated stake/accounting behind. Validate to_hotkey before lines 138-151, or make this helper transactional so receiver-validation failures roll back the mutations.
| let (amount_in, amount_out) = if order.order_type.is_buy() { | ||
| // partial fill validations have passed, it is safe here to do this | ||
| let tao_in = TaoBalance::from(signed_order.partial_fill.unwrap_or(order.amount)); | ||
| // Deduct fee from TAO input before swapping. | ||
| let fee_tao = TaoBalance::from(order.fee_rate.mul_floor(tao_in.to_u64())); | ||
| let tao_after_fee = tao_in.saturating_sub(fee_tao); | ||
|
|
||
| let alpha_out = T::SwapInterface::buy_alpha( |
There was a problem hiding this comment.
[MEDIUM] Direct buy execution can fill an order without collecting its fee
For buy orders, the pallet only passes tao_after_fee into buy_alpha, so the subtensor-side balance validation only proves the signer can pay the net swap amount. The fee is then forwarded best-effort from the signer after the swap. A signer with exactly amount - fee free TAO can have the order execute and be marked fulfilled, while forward_fee fails and leaves the fee unpaid. This makes the signed fee field unenforceable in the direct execute_orders path. Require/fund the gross buy amount atomically, or make buy fee transfer a hard precondition before marking the order executed.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| /// Thin orchestrator for `execute_batched_orders`. | ||
| fn do_execute_batched_orders( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
do_execute_batched_orders performs multiple fallible mutations in sequence: it first pulls buyer TAO and seller stake into the pallet account, then swaps, distributes outputs, writes order status, and finally forwards fees. Because the function has no transaction boundary, any failure after an earlier transfer leaves those earlier asset movements committed while the extrinsic returns an error. A malformed batch can therefore strand funds/stake in the pallet intermediary or partially settle only some orders. Wrap the whole batch execution in a FRAME transaction, or otherwise prove every later fallible operation before moving assets.
| /// Thin orchestrator for `execute_batched_orders`. | |
| fn do_execute_batched_orders( | |
| /// Thin orchestrator for `execute_batched_orders`. | |
| #[frame_support::transactional] | |
| fn do_execute_batched_orders( |
| )?; | ||
|
|
||
| // Forward the fee TAO to the order's fee recipient. | ||
| Self::forward_fee(&order.signer, &order.fee_recipient, fee_tao); |
There was a problem hiding this comment.
[MEDIUM] Direct buy execution can fill an order without collecting its fee
The buy path subtracts fee_tao from the signed amount and calls buy_alpha with only tao_after_fee. buy_alpha(..., validate: true) therefore only checks and removes the net amount from the signer. The fee transfer is attempted afterward through forward_fee, which swallows transfer failures, so a signer with enough balance for the net swap but not the fee can still receive alpha and have the order marked fulfilled without paying the signed fee. Collect/escrow the gross TAO up front, or make fee collection a mandatory part of a transaction-wrapped single-order execution.
|
🔄 AI review updated — Skeptic: VULNERABLE |
Description
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.